Codecov Report

❌ Patch coverage is 51.35135% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 44.07%. Comparing base (5651cb4) to head (1f50fc9).

@@             Coverage Diff             @@
##           develop     #921      +/-   ##
===========================================
+ Coverage    44.03%   44.07%   +0.03%     
===========================================
  Files          196      196              
  Lines        16815    16850      +35     
===========================================
+ Hits          7405     7426      +21     
- Misses        8623     8632       +9     
- Partials       787      792       +5     

@claude review PR

Claude finished @XinyuCRO's task in 3m 35s —— View job

PR Review: fix: enforce floor-data-gas

• Gather context and read changed files
• Analyze code quality and correctness
• Check for consensus/state-breaking changes
• Check for security implications
• Post review feedback

Summary

This PR enforces EIP-7623 (Prague calldata floor data gas) in all execution contexts — not just CheckTx. Previously, the guard in VerifyFee was gated on isCheckTx, meaning DeliverTx and eth_call/eth_estimateGas paths silently bypassed the floor. The fix is sound in intent and well-structured, but there is one correctness divergence from go-ethereum worth addressing.

State-Breaking Change ⚠️

Yes — this is a consensus-breaking change.

ConsensusVersion is bumped from 7 → 8 and a v8 migration is registered in module.go. The migration itself is a no-op (no store keys are written), but the version bump correctly signals validators must upgrade. This is the right approach. Do not merge this into a release branch without an upgrade handler.

Issues Found

🔴 Medium: Post-execution floor checks temporaryGasUsed (pre-refund) instead of post-refund gas used

In both x/evm/keeper/state_transition.go and x/evm/keeper/simulate.go, the post-execution floor is applied like this:

temporaryGasUsed := msg.GasLimit - leftoverGas   // BEFORE refunds
refund := GasToRefund(...)
leftoverGas += refund                              // refunds added back

if temporaryGasUsed < floorDataGas {              // ← checks PRE-refund value
    temporaryGasUsed = floorDataGas
    leftoverGas = msg.GasLimit - floorDataGas
}

In go-ethereum's EIP-7623 implementation, the floor is applied to post-refund gas used. The current code misses the case where:

• temporaryGasUsed >= floorDataGas (floor check passes), but
• temporaryGasUsed - refund < floorDataGas (post-refund gas is actually below floor)

This can occur whenever floorDataGas ≤ temporaryGasUsed < 1.25 × floorDataGas (since EIP-3529 caps refund at gasUsed / 5). In those cases, go-ethereum would charge floorDataGas, but this implementation would charge less — a consensus divergence.

Fix:

// state_transition.go / simulate.go — apply floor AFTER refund
actualGasUsed := msg.GasLimit - leftoverGas   // post-refund
if rules.IsPrague {
    floorDataGas, err := core.FloorDataGas(msg.Data)
    if err != nil {
        return nil, errorsmod.Wrap(err, "floor data gas")
    }
    if actualGasUsed < floorDataGas {
        actualGasUsed = floorDataGas
        leftoverGas = msg.GasLimit - floorDataGas
    }
}
// then use actualGasUsed instead of temporaryGasUsed going forward

Fix this →

🟡 Minor: FloorDataGas called twice per execution

core.FloorDataGas(msg.Data) is called twice in both ApplyMessageWithConfig and applyCall — once for the pre-execution check and once for the post-execution floor. This iterates over msg.Data twice. Computing it once and passing it down is cleaner and slightly more efficient.

🟡 Minor: Misleading variable name in handler_test.go

In handler_test.go, the variable is named intrinsicGas but is assigned the result of core.FloorDataGas(...). Under Prague, FloorDataGas ≥ intrinsicGas, so the value is correct functionally, but the name is misleading.

// handler_test.go — rename for clarity
floorGas, err := core.FloorDataGas(transferData)
suite.Require().NoError(err)
intrinsicGas := floorGas  // under Prague, floor >= intrinsic

🟡 Minor: Hardcoded gas constants in tests

state_transition_test.go and integration_test.go both hard-code the gas values:

intrinsicGas := uint64(21000 + 16*1024) // 37384
floorDataGas := uint64(21000 + 10*4*1024) // 61960

These will silently become stale if gas cost constants change in go-ethereum. Deriving them via core.FloorDataGas(calldata) and k.GetEthIntrinsicGas(...) would be more resilient.

🟢 CHANGELOG stanza

The entry in CHANGELOG.md is present but unlabeled. Given this is a consensus-breaking change, it should appear under a "State Machine Breaking" stanza:

### State Machine Breaking
* (evm) [#921](...) fix: enforce floor-data-gas

What's Good ✅

• VerifyFee change (utils.go) correctly removes the isCheckTx && guard — the floor must apply in all phases.
• Pre-execution check in ApplyMessageWithConfig correctly protects eth_call/eth_estimateGas paths that bypass the ante handler.
• The v8 migration being a no-op with a version bump is the correct pattern for a logic-only consensus change.
• Integration tests cover both CheckTx and DeliverTx rejection paths, which is exactly what this fix needed.
• The simulate.go changes mirror ApplyMessageWithConfig, keeping the two paths consistent.

@claude review PR

Claude finished @XinyuCRO's task in 5m 23s —— View job

PR Review: fix: enforce floor-data-gas

• Gather context from PR metadata
• Read changed files
• Analyze correctness and consensus implications
• Post inline comments on issues
• Final review summary

Summary

This PR enforces EIP-7623 (Prague calldata floor data gas) in all execution contexts, not just CheckTx. The core intent is correct and the structure is sound. The previous code had the floor check in VerifyFee gated on isCheckTx, which silently bypassed it during FinalizeBlock / eth_call / eth_estimateGas. The one-line fix in utils.go (- isCheckTx &&) is the crux of the change; everything else adds defense-in-depth enforcement and tests.

State-Breaking Change ⚠️

Yes — this is a consensus-breaking change. ConsensusVersion is bumped 7 → 8 and a v8 migration is registered in module.go. The migration itself is a no-op (no store keys written), which is the correct pattern for a logic-only consensus change — validators must upgrade to maintain consensus. Do not merge without an upgrade handler in the consuming app.

Issues Found

🟡 Minor: intrinsicGas variable name is misleading in handler_test.go

In handler_test.go lines 423 and 527, the variable is named intrinsicGas but holds the result of core.FloorDataGas(...). Under Prague, FloorDataGas ≥ IntrinsicGas, so the value is functionally correct for use as a gasLimit, but the name is wrong and creates confusion about which gas concept is being exercised.

// Before
intrinsicGas, err := core.FloorDataGas(transferData)

// After — clearer
floorGas, err := core.FloorDataGas(transferData)

The test comment at line 434 also says "enough for intrinsicGas, but not enough for execution" — it should say "enough for floorDataGas, but not enough for execution".

🟡 Minor: Hardcoded gas constants in state_transition_test.go

intrinsicGas := uint64(21000 + 16*1024) // 37384
floorDataGas := uint64(21000 + 10*4*1024) // 61960

These will silently become stale if upstream gas cost constants change. Deriving them via core.FloorDataGas(calldata) and k.GetEthIntrinsicGas(...) (as done in handler_test.go) would be more resilient.

🟡 Minor: CHANGELOG stanza placement

The new CHANGELOG entry is placed under the flat ## Unreleased section. Given this is a consensus-breaking change, it belongs under the explicit ### State Machine Breaking stanza as required by the CHANGELOG guidelines at the top of the file:

### State Machine Breaking
* (evm) [#921](https://github.com/crypto-org-chain/ethermint/pull/921) fix: enforce floor-data-gas

🟢 MaxUsedGas in simulate.go is still correct

applyCallResult.MaxUsedGas is intentionally the pre-refund peak gas (temporaryGasUsed), matching go-ethereum's ExecutionResult.MaxUsedGas semantics. The GasUsed field (post-refund, post-floor) is the one used for billing. This is fine.

What's Good ✅

• utils.go — removing isCheckTx && is the minimal, correct fix. The floor now applies in all phases uniformly.
• Pre-execution check in ApplyMessageWithConfig (line 419–428) and applyCall correctly protect eth_call/eth_estimateGas paths that bypass the ante handler.
• Post-execution floor in both state_transition.go and simulate.go correctly operates on post-refund gas (msg.GasLimit - leftoverGas after leftoverGas += refund), which matches EIP-7623 / go-ethereum semantics. This correctly handles the case where gasUsed drops below the floor after refunds are applied.
• No double-computation concern — FloorDataGas is called twice per execution (pre- and post-check) but the data is small and this is not a hot path that warrants optimization.
• v8 migration is correctly a no-op with a version bump.
• Integration tests cover CheckTx and DeliverTx rejection paths, which is exactly what this fix needed.
• handler_test.go updates correctly replaced the hardcoded 21572 / 134510 with derived values — the hardcoded numbers were wrong under Prague rules.

Is there a way to fix VerifyFee() to have this check also in execute tx for early cheap defense?

Is there a way to fix VerifyFee() to have this check also in execute tx for early cheap defense?

this check works on both CheckTx and FinalizeBlock now